Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow pytest --migrations to succeed #14663

Conversation

chrismeyersfsu
Copy link
Member

@chrismeyersfsu chrismeyersfsu commented Nov 15, 2023

SUMMARY

We actually subvert migrations from running in test via pytest.ini --no-migrations option. This has led to bit rot for the sqlite migrations happy path. This changeset pays off that tech debt and allows for an sqlite migration happy path.

  • This paves the way for programatic invocation of individual migrations and weaving of the creation of resources (i.e. Instance, Job Template, etc). With this, a developer can instantiate various database states, trigger a migration, assert the state of the db, and then have pytest rollback all of that.

  • I will note that in practice, running these migrations is dog shit slow BUT this work also opens up the possibility of saving and re-using sqlite3 database files. Normally, caching is not THE answer and causes more harm than good. But in this case, our migrations are mostly write-once (I say mostly because this change set violates that :) so cache invalidation isn't a major issue.ge does.

  • Adds new ci test api-migration

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API
AWX VERSION

ADDITIONAL INFORMATION

To test this. Run the below without these changes. See everything blow up?. Now run the tests with these changes ... success.

pytest --migrations

Note: you can also just run like 1 test. You just want to trigger the running of migrations to bring up the test db. Oh, and you might need to delete the test db file rm -f awx/awx_test.sqlite3

@chrismeyersfsu chrismeyersfsu force-pushed the fix_migrations_so_they_are_runnable_in_testing branch 2 times, most recently from 48b5144 to 7268e46 Compare November 15, 2023 19:50
@chrismeyersfsu chrismeyersfsu force-pushed the fix_migrations_so_they_are_runnable_in_testing branch 2 times, most recently from a5fa112 to a80ce92 Compare November 15, 2023 21:01
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Nov 15, 2023
@chrismeyersfsu chrismeyersfsu force-pushed the fix_migrations_so_they_are_runnable_in_testing branch 6 times, most recently from fb42b7b to a0c3490 Compare November 16, 2023 12:48

def database_forwards(self, app_label, schema_editor, from_state, to_state):
if not schema_editor.connection.vendor.startswith('postgres'):
self.code = self.sqlite_code or migrations.RunPython.noop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this behavior. One point, months ago, I did the same thing you did here - made the migrations compatible with sqlite3. But for the life of me I can not find the branch where I worked on that.

I just added a condition in the migration logic depending on the vendor, and that didn't take as many changes as you could think. Sure there are lots of logic steps we just don't want to run, but doing this I expect we're going to miss some stuff we should have, like creating default system jobs, or creating default credential types.

Also the naming of sqlitemigrations this is kind of concerning when taken out of this context.

Copy link
Member Author

@chrismeyersfsu chrismeyersfsu Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow.

Do you not like that the user can supply None for sqlite_code and are advocating for requiring sqlite_code to always be provided?
OR
Do you not like the overall idea of postgres sql vs. sqlite sql?
OR
Do you not like the variable choice that we are pivoting on?
OR
Do you not like "burying" this pivot logic and would rather the pivot be directly in the developers face, in the migration?

I'm fine with renaming sqlitemigrations

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked with @AlanCoding offline.

TL;DR self.code = self.sqlite_code or self.code or migrations.RunPython.noop instead of self.code = self.sqlite_code or migrations.RunPython.noop 👍

@chrismeyersfsu chrismeyersfsu force-pushed the fix_migrations_so_they_are_runnable_in_testing branch 2 times, most recently from fb95943 to 341434f Compare November 16, 2023 14:32
@chrismeyersfsu chrismeyersfsu force-pushed the fix_migrations_so_they_are_runnable_in_testing branch 3 times, most recently from 4612d53 to a664082 Compare November 16, 2023 18:14
* We actually subvert migrations from running in test via pytest.ini
  --no-migrations option. This has led to bit rot for the sqlite
  migrations happy path. This changeset pays off that tech debt and
  allows for an sqlite migration happy path.
* This paves the way for programatic invocation of individual migrations
  and weaving of the creation of resources (i.e. Instance, Job Template,
  etc). With this, a developer can instantiate various database states,
  trigger a migration, assert the state of the db, and then have pytest
  rollback all of that.
* I will note that in practice, running these migrations is dog shit
  slow BUT this work also opens up the possibility of saving and
  re-using sqlite3 database files. Normally, caching is not THE answer
  and causes more harm than good. But in this case, our migrations are
  mostly write-once (I say mostly because this change set violates
  that :) so cache invalidation isn't a major issue.
@chrismeyersfsu chrismeyersfsu force-pushed the fix_migrations_so_they_are_runnable_in_testing branch from a0ee538 to 9ad2239 Compare November 16, 2023 20:34
@@ -24,6 +26,11 @@ def migrate_event_data(apps, schema_editor):
cursor.execute(f'ALTER TABLE {tblname} ALTER COLUMN id TYPE bigint USING id::bigint;')


def migrate_event_data_sqlite(apps, schema_editor):
# TODO: cmeyers fill this in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still got a TODO here. Why? Are you really going to migrate event data? I see no way you will get the hourly event tables to work in sqlite3. Of course it doesn't support range partitioning from the get-go, but even if you created superficial "partitions" as separate tables, I'm skeptical that it could be made to work, or that it would be worth it if you could do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is for the BigInt migration. I'm sure we won't ever want to do this in sqlite3. The next file does migration to partitions, which again, may be feasible superficially, but questionable value.

@@ -24,6 +26,11 @@ def migrate_event_data(apps, schema_editor):
cursor.execute(f'ALTER TABLE {tblname} ALTER COLUMN id TYPE bigint USING id::bigint;')


def migrate_event_data_sqlite(apps, schema_editor):
# TODO: cmeyers fill this in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is something needed here?

"""
ALTER TABLE main_activitystream RENAME setting TO setting_old;
ALTER TABLE main_activitystream ALTER COLUMN setting_old DROP NOT NULL;
""",
sqlite_sql="ALTER TABLE main_activitystream RENAME setting TO setting_old",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I guess this is kind of nice.

@chrismeyersfsu
Copy link
Member Author

I might should remove the todo. I’m not going to do it in this or.

A noop seems fine for the cases I have a todo for the time being. If someone makes a change that requires testing code and data around those todo then they may have to pay the penalty of making them work in SQLite.

def database_backwards(self, app_label, schema_editor, from_state, to_state):
if not schema_editor.connection.vendor.startswith('postgres'):
self.reverse_sql = self.sqlite_reverse_sql or migrations.RunSQL.noop
super().database_forwards(app_label, schema_editor, from_state, to_state)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this intended?

Suggested change
super().database_forwards(app_label, schema_editor, from_state, to_state)
super().database_backwards(app_label, schema_editor, from_state, to_state)

Copy link
Member Author

@chrismeyersfsu chrismeyersfsu Nov 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, not intentional. Thank you!

def database_backwards(self, app_label, schema_editor, from_state, to_state):
if not schema_editor.connection.vendor.startswith('postgres'):
self.reverse_code = self.sqlite_reverse_code or migrations.RunPython.noop
super().database_forwards(app_label, schema_editor, from_state, to_state)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
super().database_forwards(app_label, schema_editor, from_state, to_state)
super().database_backwards(app_label, schema_editor, from_state, to_state)

* We commonly subvert running migrations in test land. Test land uses
  sqlite. By not constantly exercising this code path it atrophies. The
  smoke test here is to continuously exercise that code path.
* Add ci test to run migration tests separately, they take =~ 2-3
  minutes each on my laptop.
* The smoke tests also serves as an example of how to write migration
  tests.
@chrismeyersfsu chrismeyersfsu force-pushed the fix_migrations_so_they_are_runnable_in_testing branch from 9ad2239 to 3d040c3 Compare November 17, 2023 18:03
@chrismeyersfsu chrismeyersfsu merged commit 2ac304d into ansible:devel Nov 17, 2023
21 checks passed
djyasin added a commit to djyasin/awx that referenced this pull request Jan 4, 2024
Fixed ldap url a bit and cleaned up a few things.

Changed conditional back to adn rather than or and changed var back to password rather than username.

fix wsrelay connection in ipv6 environments

Fix: ansible#14511 Add alt-text codeblock to Images for Userguide: main_menu.rst

Signed-off-by: Ratan Gulati <[email protected]>

Revised the proposed Alt text for main_menu.rst

Update insights.rst

Update insights.rst

Revised proposed alt text for insights.rst

Updated images - Workflow Templates chapter of Userguide.

Removed references to images that were deleted.

Fix: ansible#14523 Add alt-text codeblock to Images for workflow_template.rst (ansible#14604)

* add alt to images in workflow_templates.rst

Signed-off-by: Ratan Gulati <[email protected]>

* add alt to images in workflow_templates.rst

Signed-off-by: Ratan Gulati <[email protected]>

* Update workflow_templates.rst

* Revised proposed alt text for workflow_templates.rst

---------

Signed-off-by: Ratan Gulati <[email protected]>
Co-authored-by: TVo <[email protected]>

Added alt text for settings-menu.rst (ansible#14639)

* Re-do for PR ansible#14595 to fix CI issues.

* Added alt text to settings-menu.rst

* Update docs/docsite/rst/common/settings-menu.rst

Co-authored-by: Don Naro <[email protected]>

---------

Co-authored-by: Hao Liu <[email protected]>
Co-authored-by: Don Naro <[email protected]>

Docs: Include REST API reference content from swagger.json (ansible#14607)

Setting credential_type as required (ansible#14651)

* Setting credential_type as required

* Added test for missing credential_type in credential module

* Corrected test assertion

---------

Co-authored-by: Lucas Benedito <[email protected]>

Replaced with larger graphic. (ansible#14647)

Correctly handle case where unpartitioned table does not exist (ansible#14648)

Set subscription type as developer for developer subscriptions. (ansible#14584)

* Set subscription type as developer for developer subscriptions.

Signed-off-by: Tong He <[email protected]>

* Set subscription type as developer for developer subscription manifests.

Signed-off-by: Tong He <[email protected]>

* Remedy the wrong character to assign value.

Signed-off-by: Tong He <[email protected]>

* Reformat licensing.py by black.

Signed-off-by: Tong He <[email protected]>

---------

Signed-off-by: Tong He <[email protected]>

issue ansible#14653 heading does not render correctly (ansible#14665)

Adding the possibility to decode base64 decoded strings to Delinea's Devops Secret Vault (DSV) (ansible#14646)

Adding the possibility to decode base64 decoded strings to Delinea's Devops Secret Vault (DSV).
This is necessary as uploading files to DSV is not possible (and not meant to be) and files should be added base64 encoded.
The commit is making sure to remain backward compatible (no secret decoding), as a default is supplied.

This has been tested with DSV and works for secrets that are base64 encoded and secrets that are not base64 encoded (which is the default).

Signed-off-by: Steffen Scheib <[email protected]>

Added missing pointers to release notes (ansible#14659)

* Replaced with larger graphic.

* Revert "Replaced with larger graphic."

This reverts commit 1214b00.

* Added missing pointers to release notes.

Make vault init more idempotent (ansible#14664)

Currently if you cleanup docker volume for vault and bring docker-compose development back up with vault enabled we will not initialize vault because the secret files still exist.

This change will attempt to initialize vault reguardless and update the secret file if vault is initialized

Remove unused methods we attach to user model (ansible#14668)

Update RBAC docs, remove unused get_permissions (ansible#14492)

* Update RBAC docs, remove unused get_permissions

* Add back in section for get_roles_on_resource

Upgrade doc requirements (ansible#14669)

* upgrade when pip compiling doc reqs

* upgrade doc requirements

allow pytest --migrations to succeed (ansible#14663)

* allow pytest --migrations to succeed

* We actually subvert migrations from running in test via pytest.ini
  --no-migrations option. This has led to bit rot for the sqlite
  migrations happy path. This changeset pays off that tech debt and
  allows for an sqlite migration happy path.
* This paves the way for programatic invocation of individual migrations
  and weaving of the creation of resources (i.e. Instance, Job Template,
  etc). With this, a developer can instantiate various database states,
  trigger a migration, assert the state of the db, and then have pytest
  rollback all of that.
* I will note that in practice, running these migrations is dog shit
  slow BUT this work also opens up the possibility of saving and
  re-using sqlite3 database files. Normally, caching is not THE answer
  and causes more harm than good. But in this case, our migrations are
  mostly write-once (I say mostly because this change set violates
  that :) so cache invalidation isn't a major issue.

* functional test for migrations on sqlite

* We commonly subvert running migrations in test land. Test land uses
  sqlite. By not constantly exercising this code path it atrophies. The
  smoke test here is to continuously exercise that code path.
* Add ci test to run migration tests separately, they take =~ 2-3
  minutes each on my laptop.
* The smoke tests also serves as an example of how to write migration
  tests.

* run migration tests in ci

Fix awx collection publishing on galaxy (ansible#14642)

--location (-L) parameter will prompt curl to submit a new request if the URL is a redirect.

After moving to galaxy-NG without -L the curl falsely return 302 for any version

Co-authored-by: John Barker <[email protected]>

Fixing wsrelay connection loop (ansible#14692)

* Fixing wsrelay connection loop

* The loop was being interrupted when reaching the return statements, causing a race condition that would make nodes remain disconnected from their websockets
* Added log messages for the previous return state to improve the logging from this state.

* Added logging for malformed payload

* Update awx/main/wsrelay.py

Co-authored-by: Rick Elrod <[email protected]>

* Moved logmsg outside condition

---------

Co-authored-by: Lucas Benedito <[email protected]>
Co-authored-by: Rick Elrod <[email protected]>

Fix the bulk Job Launch Integration test in awx collection (ansible#14702)

* fix the integration tests

[CI] Reduce GHA timeouts from 6h default (ansible#14704)

* [CI] Reduce GHA timeouts from 6h default

The goal here is to never interfere with a real run (so most of the
timeout-minutes values seem rather high) but to avoid having 6h long
runs if something goes crazy and never ends.

Signed-off-by: Rick Elrod <[email protected]>

* Do bash hackery instead

Signed-off-by: Rick Elrod <[email protected]>

---------

Signed-off-by: Rick Elrod <[email protected]>

separate tox calls in readthedocs config (ansible#14673)

Add TLS certificate auth for HashiCorp Vault (ansible#14534)

* Add TLS certificate auth for HashiCorp Vault

Add support for AWX to authenticate with HashiCorp Vault using
TLS client certificates.

Also updates the documentation for the HashiCorp Vault secret management
plugins to include both the new TLS options and the missing Kubernetes
auth method options.

Signed-off-by: Andrew Austin <[email protected]>

* Refactor docker-compose vault for TLS cert auth

Add TLS configuration to the docker-compose Vault configuration and
use that method by default in vault plumbing.

This ensures that the result of bringing up the docker-compose stack
with vault enabled and running the plumb-vault playbook is a fully
working credential retrieval setup using TLS client cert authentication.

Signed-off-by: Andrew Austin <[email protected]>

* Remove incorrect trailing space

Co-authored-by: Hao Liu <[email protected]>

* Make vault init idempotent

- improve error handling for vault_initialization
- ignore error if vault cert auth is already configured
- removed unused register

* Add VAULT_TLS option

Make TLS for HashiCorp Vault optional and configurable via VAULT_TLS env var

* Add retries for vault init

Sometime it took longer for vault to fully come up and init will fail

---------

Signed-off-by: Andrew Austin <[email protected]>
Co-authored-by: Hao Liu <[email protected]>
Co-authored-by: Hao Liu <[email protected]>

remove unnecessary required flags for saml backend (ansible#14666)

Signed-off-by: Tyler Muir <[email protected]>

Dependabot for docsite requirements (ansible#14670)

Add django-ansible-base (ansible#14705)

* add django-ansible-base

Signed-off-by: jessicamack <[email protected]>

* add licenses

* add django-ansible-base

Signed-off-by: jessicamack <[email protected]>

* add licenses

* apply patch to fix permissions issue

---------

Signed-off-by: jessicamack <[email protected]>

Remove incorrectly formatted line from requirements.txt (ansible#14714)

remove git+ line

Fix updater bug due to missing newline at EOF (ansible#14713)

Fix undefined error in settings/logging/edit form (ansible#14715)

Fix undefined error in logging settings edit form

Update setuptools-scm (ansible#14716)

* properly format requirement

* upgrade setuptools_scm

* Revert "properly format requirement"

This reverts commit 4c87929.

* test ansible-runner package upgrade

* Revert "test ansible-runner package upgrade"

This reverts commit ba4b74f.

Adding hosts bulk deletion feature (ansible#14462)

* Adding hosts bulk deletion feature

Signed-off-by: Avi Layani <[email protected]>

* fix the type of the argument

Signed-off-by: Avi Layani <[email protected]>

* fixing activity_entry tracking

Signed-off-by: Avi Layani <[email protected]>

* Revert "fixing activity_entry tracking"

This reverts commit c8eab52.
Since the bulk_delete is not related to an inventory, only hosts which
can be from different inventories.

* get only needed vars to reduce memory consumption

Signed-off-by: Avi Layani <[email protected]>

* filtering the data to reduce memory increase the number of queries

Signed-off-by: Avi Layani <[email protected]>

* update the activity stream for inventories

Signed-off-by: Avi Layani <[email protected]>

* fix the changes dict initialiazation

Signed-off-by: Avi Layani <[email protected]>

---------

Signed-off-by: Avi Layani <[email protected]>

Remove superwatcher from docker-compose dev (ansible#14708)

When making changes to the application sometime you can accidentally cause FATAL state and cause the dev container to crash which will remove any ephemeral changes that you have made and is ANNOYING!

Recover rsyslog from 4xx error

Due to ansible#7560

'omhttp' module for rsyslog will completely stop forwarding message to external log aggregator after receiving a 4xx error from the external log aggregator

This PR is an "workaround" for this problem by restarting rsyslogd after detecting that rsyslog received a 4xx error

Correct misuse of stdxxx_event_enabled

Not every log messages need to be emitted as a event!

Send SIGKILL to rsyslog if hard cancellation is needed

Simplify RBAC get_roles_on_resource method (ansible#14710)

* Simplify RBAC get_roles_on_resource method

* Fix bug

* Fix query type bug

Narrow the actor types accepted for RBAC evaluations (ansible#14709)

* Narrow the scope of RBAC evaluations

* Update tests for RBAC method changes

* Simplify querset for credentials in org

* Fix call pattern to pass in team role obj

Use filtering/sorting from django-ansible-base (ansible#14726)

* Move filtering to DAB

* add comment to trigger building a new image

Signed-off-by: jessicamack <[email protected]>

* remove unneeded comment

Signed-off-by: jessicamack <[email protected]>

* remove unused imports

Signed-off-by: jessicamack <[email protected]>

* change mock import

Signed-off-by: jessicamack <[email protected]>

---------

Signed-off-by: jessicamack <[email protected]>
Co-authored-by: jessicamack <[email protected]>

add awx collection export tests

* Basic export tests
* Added test that highlights a problem with running Schedule exports as
  non-root user. We rely on the POST key in the OPTIONS response to
  determine the fields to export for a resource. The POST key is not
  present if a user does NOT have create privileges.
* Fixed up forwarding all headers from the API server back to the test
  code. This was causing a problem in awxkit code that checks for
  allowed HTTP Verbs in the headers.

refactor awxkit import code

* Move awxkit import code into a pytest fixture to better control when
  the import happens
* Ensure /awx_devel/awxkit is added to sys path before awxkit import
  runs

Fix twilio_backend.py to send SMS to multiple destinations. (ansible#14656)

AWX only sends Twilio notifications to one destination with the current version of code, but this is a bug. Fixed this bug for sending SMS to multiple destinations.

Persist schedule prompt on launch fields when editing (ansible#14736)

* persist schedule prompt on launch fields when editing

* Merge job template default credentials with schedule overrides in schedule prompt

* rename vars for clarity

* handle undefined defaultCredentials

---------

Co-authored-by: Michael Abashian <[email protected]>
djyasin pushed a commit to djyasin/awx that referenced this pull request Sep 16, 2024
* allow pytest --migrations to succeed

* We actually subvert migrations from running in test via pytest.ini
  --no-migrations option. This has led to bit rot for the sqlite
  migrations happy path. This changeset pays off that tech debt and
  allows for an sqlite migration happy path.
* This paves the way for programatic invocation of individual migrations
  and weaving of the creation of resources (i.e. Instance, Job Template,
  etc). With this, a developer can instantiate various database states,
  trigger a migration, assert the state of the db, and then have pytest
  rollback all of that.
* I will note that in practice, running these migrations is dog shit
  slow BUT this work also opens up the possibility of saving and
  re-using sqlite3 database files. Normally, caching is not THE answer
  and causes more harm than good. But in this case, our migrations are
  mostly write-once (I say mostly because this change set violates
  that :) so cache invalidation isn't a major issue.

* functional test for migrations on sqlite

* We commonly subvert running migrations in test land. Test land uses
  sqlite. By not constantly exercising this code path it atrophies. The
  smoke test here is to continuously exercise that code path.
* Add ci test to run migration tests separately, they take =~ 2-3
  minutes each on my laptop.
* The smoke tests also serves as an example of how to write migration
  tests.

* run migration tests in ci
djyasin pushed a commit to djyasin/awx that referenced this pull request Nov 11, 2024
* allow pytest --migrations to succeed

* We actually subvert migrations from running in test via pytest.ini
  --no-migrations option. This has led to bit rot for the sqlite
  migrations happy path. This changeset pays off that tech debt and
  allows for an sqlite migration happy path.
* This paves the way for programatic invocation of individual migrations
  and weaving of the creation of resources (i.e. Instance, Job Template,
  etc). With this, a developer can instantiate various database states,
  trigger a migration, assert the state of the db, and then have pytest
  rollback all of that.
* I will note that in practice, running these migrations is dog shit
  slow BUT this work also opens up the possibility of saving and
  re-using sqlite3 database files. Normally, caching is not THE answer
  and causes more harm than good. But in this case, our migrations are
  mostly write-once (I say mostly because this change set violates
  that :) so cache invalidation isn't a major issue.

* functional test for migrations on sqlite

* We commonly subvert running migrations in test land. Test land uses
  sqlite. By not constantly exercising this code path it atrophies. The
  smoke test here is to continuously exercise that code path.
* Add ci test to run migration tests separately, they take =~ 2-3
  minutes each on my laptop.
* The smoke tests also serves as an example of how to write migration
  tests.

* run migration tests in ci
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:api dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants